Implement parsing for records #40467

agocke · 2019-12-18T05:45:42Z

I recommend reviewing the first commit separately. All it does is split DeclarationModifiers into its own field in our flags structs since we ran out of space, and splitting the modifiers out was simpler and clearer. The second commit actually implements the parsing.

As agreed in LDM, the specification requires that a data contextual
keyword be present in front of the class and a parameter list to be
present after the identifier, for the type to be a record.

The absence of either of these two elements will be a semantic error,
not a parsing error.

Relates to #40726 (test plan for records)

As agreed in LDM, the specification requires that a data contextual keyword be present in front of the class and a parameter list to be present after the identifier, for the type to be a record. The absence of either of these two elements will be a semantic error, not a parsing error.

333fred · 2019-12-27T23:01:19Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

            //
-            // | |d|yy|xxxxxxxxxxxxxxxxxxxxxxx|wwwwww|
+            // |               |vvv|zzzz|f|d|yy||wwwwww|


| [](start = 48, length = 1)

Extra vertical bar #Closed

gafter · 2019-12-27T23:33:29Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -1485,6 +1527,8 @@ private TypeDeclarationSyntax ParseClassOrStructOrInterfaceDeclaration(SyntaxLis
                            semicolon);

                    case SyntaxKind.InterfaceKeyword:
+                        RoslynDebug.Assert(openBrace != null);


RoslynDebug [](start = 24, length = 11)

I don't see how this assertion is correct. It appears that the above code would accept interface A; I think we either need to make the curly braces optional or make missing tokens for them with an error. #Closed

Similarly this code appears to be dropping the parameter list on the floor. I think it needs to be either added to the syntax, not permitted by the parser, or you need to build a class declaration when that happens.

In reply to: 361761780 [](ancestors = 361761780)

gafter · 2019-12-27T23:42:10Z

src/Compilers/CSharp/Portable/Syntax/ClassDeclarationSyntax.cs

+                parameterList,
+                baseList,
+                constraintClauses,
+                openBraceToken: default,


default [](start = 32, length = 7)

I suspect you intended this to be the creation of a fresh semicolon token if there were no members, or fresh brace tokens if there were. #Closed

However, since you don't know the language version at this point, perhaps this should create brace tokens always.

In reply to: 361762456 [](ancestors = 361762456)

gafter · 2019-12-27T23:43:46Z

src/Compilers/CSharp/Portable/Syntax/StructDeclarationSyntax.cs

+                default,
+                members,
+                default,
+                default);


default [](start = 16, length = 7)

I suspect you intended this to be the creation of a fresh semicolon token if there were no members, or fresh brace tokens if there were.

gafter · 2019-12-27T23:47:39Z

src/Compilers/CSharp/Test/Emit/Emit/EndToEndTests.cs

@@ -156,7 +156,7 @@ public void DeeplyNestedGeneric()
                (ExecutionArchitecture.x86, ExecutionConfiguration.Debug) => 270,
                (ExecutionArchitecture.x86, ExecutionConfiguration.Release) => 1290,
                (ExecutionArchitecture.x64, ExecutionConfiguration.Debug) => 170,
-                (ExecutionArchitecture.x64, ExecutionConfiguration.Release) => 730,
+                (ExecutionArchitecture.x64, ExecutionConfiguration.Release) => 700, // PROTOTYPE(angocke): Improve stack usage


// PROTOTYPE(angocke): Improve stack usage [](start = 84, length = 42)

I wouldn't bother.

gafter · 2019-12-27T23:48:51Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+    public class RecordTests : CompilingTestBase
+    {
+        [Fact]
+        public void RecordLanguageVersion()


RecordLanguageVersion [](start = 20, length = 21)

Also try class Point;. That should not be permitted in older language versions either.

gafter · 2019-12-27T23:52:06Z

Do you have a document that describes the feature this is intended to implement? Neither https://github.com/dotnet/csharplang/blob/master/proposals/records.md nor https://github.com/dotnet/csharplang/blob/master/proposals/recordsv2.md seems to be right.

gafter · 2019-12-27T23:56:30Z

One of the things I'd be looking for in the specification is how you will modify the syntax and parser to handle constructor parameters of the base type. Without knowing how that's going to be handled, I cannot know if these parser changes implement the intended design.

In reply to: 569365350 [](ancestors = 569365350)

gafter · 2019-12-27T23:59:23Z

Finished review pass (Iteration 2).

333fred · 2019-12-31T18:52:23Z

eng/targets/Settings.props

@@ -17,7 +17,7 @@
    <DisableImplicitNuGetFallbackFolder>true</DisableImplicitNuGetFallbackFolder>
    <ToolsetPackagesDir>$(RepoRoot)build\ToolsetPackages\</ToolsetPackagesDir>

-    <RoslynPortableTargetFrameworks>net472;netcoreapp2.1</RoslynPortableTargetFrameworks>
+    <RoslynPortableTargetFrameworks>netcoreapp2.1;net472</RoslynPortableTargetFrameworks>


Please revert this change. Quoting from @jaredpar a few months ago:

We need to keep net472 as the first TF listed because that controls items like which tests run by default.

Yeah, that's the point. We should swap the default. I can't test on Mac using VS Code unless it's first.

We should swap the default. I can't test on Mac using VS Code unless it's first.

Definitely don't want us to do this. As @333fred noted, the order here is deliberate. Changing it will not fix anything, instead it will just fix one scenario and break another. In this case it would be fixing Mac, the minority scenario, and breaking Windows, the dominant scenario.

Where is the bug on VS Code that prevents Mac testing here? Have you filed a bug?

Why would it break Windows? The test explorer seems to work fine. Doesn't look like there's a bug here for VS Code. They don't support debugging on anything but .NET Core, and switching between target frameworks isn't a bug, it's a feature they don't have.

Why would it break Windows?

The set of tests run for desktop and core are significantly different. Moving to core by default will break the expectations of developers that expect to get the desktop tests.

If you think the trade off is a good one here then the correct course of action is to discuss the policy with the team. Not change it subtly in a PR. Particularly since this has come up several times before and we've kept the current order for these reasons.

Doesn't look like there's a bug here for VS Code.

VS Code cannot test code that every other tool in our toolbox can test. Is the explicit design of VS Code that they can't support multi-targeted projects? If so that seems really unfortunate.

it's a feature they don't have.

Where is the issue tracking the feature request?

If you think the trade off is a good one here then the correct course of action is to discuss the policy with the team. Not change it subtly in a PR. Particularly since this has come up several times before and we've kept the current order for these reasons.

Yup, this is just going into a feature branch. I had planned to send an email out before changing it in master. Can I leave a PROTOTYPE comment above and we can do that when I'm back in Seattle?

Where is the issue tracking the feature request?

One thing we have to change for that is running our desktop tests as 64-bit by default. For the life of me I cannot figure out how to change that from 32-bit. I gave up after a couple hours.

dotnet/vscode-csharp#1716

You aren't doing anything wrong. x86 debugging is not supported by the C# extension. You will need full Visual Studio for that.

Beyond that, I'm not sure what feature we would be asking for. Some way to change the framework the tests are run under? VS only got that a couple months ago, so it's not exactly something I'd put at the top of the VS Code priority list.

Can I leave a PROTOTYPE comment above and we can do that when I'm back in Seattle?

That's fine by me.

Beyond that, I'm not sure what feature we would be asking for. Some way to change the framework the tests are run under?

I think the ask is pretty straight forward: we have a project which is multi-targeted to desktop and .NET Core. When that happens don't fail, instead prefer the .NET Core target framework. Basically do the only action which is capable of succeeding in this environment.

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

333fred · 2019-12-31T19:22:37Z

Done review pass (commit 2)

333fred · 2020-01-02T17:48:51Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                SyntaxToken semicolon;
+                SyntaxToken? openBrace;
+                SyntaxToken? closeBrace;
+                if (isInterface || CurrentToken.Kind != SyntaxKind.SemicolonToken)


CurrentToken.Kind != SyntaxKind.SemicolonToken [](start = 35, length = 46)

I feel like this isn't a great solution. I know this change was prompted by Neal, but I feel like a better solution would be to parse the interface as a record declaration, and then issue a semantic error for "Interfaces cannot be records."

333fred · 2020-01-02T17:52:19Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+            comp.VerifyDiagnostics();
+            comp = CreateCompilation(src2, parseOptions: TestOptions.RegularPreview);
+            comp.VerifyDiagnostics();
+            comp = CreateCompilation(src3, parseOptions: TestOptions.RegularPreview);


Missing a VerifyDiagnostics

333fred · 2020-01-02T17:56:37Z

Done review pass (commit 5)

gafter · 2020-01-02T20:02:40Z

Similarly, I would expect to see a syntax tree and parsing for the "constructor body" from https://github.com/dotnet/csharplang/blob/master/proposals/records.md#primary-constructor-body (which incidentally was copied verbatim into Java's record feature).

In reply to: 569365766 [](ancestors = 569365766,569365350)

gafter

This looks good as a partial implementation of parsing for what I expect a record specification to look like based on recent discussions. If my expectations are wrong then I'd want to review this code again with respect to whatever the specification actually is. To save us all from having to re-review code going forward, please provide a specification of this feature.

agocke · 2020-01-03T03:39:44Z

@gafter Good point, I've added https://github.com/dotnet/csharplang/blob/master/proposals/records-wip.md

Basically, I intend to support the syntax that you linked, with the added data modifier, but I'm trying to carve out a few minimal work items to avoid making giant PRs.

agocke force-pushed the record-syntax branch 2 times, most recently from 282e10a to 6d069ea Compare December 18, 2019 23:12

agocke changed the title ~~Record syntax~~ Implement parsing for records Dec 18, 2019

agocke force-pushed the record-syntax branch 2 times, most recently from 73ed2cc to 336907b Compare December 25, 2019 05:05

Make more space for DeclarationModifiers

bb4d518

agocke force-pushed the record-syntax branch 5 times, most recently from 83ff132 to 5b6c6dd Compare December 26, 2019 21:30

agocke force-pushed the record-syntax branch from 5b6c6dd to dc69d22 Compare December 27, 2019 03:43

agocke marked this pull request as ready for review December 27, 2019 21:43

agocke requested review from a team as code owners December 27, 2019 21:43

333fred added the Area-Compilers label Dec 27, 2019

333fred reviewed Dec 27, 2019

View reviewed changes

gafter reviewed Dec 27, 2019

View reviewed changes

333fred reviewed Dec 31, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

Respond to PR comments

e1b84b1

agocke added 2 commits January 1, 2020 18:26

Respond to PR comments

2d2d451

Update test baselines

7f3601e

333fred reviewed Jan 2, 2020

View reviewed changes

Respond to PR comments

31b8fc5

gafter approved these changes Jan 2, 2020

View reviewed changes

jcouv added this to the Compiler.Net5 milestone Jan 3, 2020

333fred approved these changes Jan 3, 2020

View reviewed changes

Remove extra public API members

196d8f5

agocke merged commit 8769e0b into dotnet:features/records Jan 5, 2020

agocke deleted the record-syntax branch January 5, 2020 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement parsing for records #40467

Implement parsing for records #40467

agocke commented Dec 18, 2019 •

edited by jcouv

Loading

333fred Dec 27, 2019 •

edited

Loading

gafter Dec 27, 2019 •

edited

Loading

gafter Dec 27, 2019

gafter Dec 27, 2019 •

edited by 333fred

Loading

gafter Dec 31, 2019

gafter Dec 27, 2019 •

edited

Loading

gafter Dec 27, 2019

gafter Dec 27, 2019

gafter commented Dec 27, 2019

gafter commented Dec 27, 2019

gafter commented Dec 27, 2019

333fred Dec 31, 2019

agocke Jan 1, 2020

jaredpar Jan 2, 2020

agocke Jan 2, 2020 •

edited

Loading

jaredpar Jan 2, 2020

agocke Jan 2, 2020

jaredpar Jan 2, 2020

333fred commented Dec 31, 2019 •

edited

Loading

333fred Jan 2, 2020

333fred Jan 2, 2020

333fred commented Jan 2, 2020

gafter commented Jan 2, 2020

gafter left a comment

agocke commented Jan 3, 2020

Implement parsing for records #40467

Implement parsing for records #40467

Conversation

agocke commented Dec 18, 2019 • edited by jcouv Loading

333fred Dec 27, 2019 • edited Loading

Choose a reason for hiding this comment

gafter Dec 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Dec 27, 2019 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Dec 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Dec 27, 2019

gafter commented Dec 27, 2019

gafter commented Dec 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Dec 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jan 2, 2020

gafter commented Jan 2, 2020

gafter left a comment

Choose a reason for hiding this comment

agocke commented Jan 3, 2020

agocke commented Dec 18, 2019 •

edited by jcouv

Loading

333fred Dec 27, 2019 •

edited

Loading

gafter Dec 27, 2019 •

edited

Loading

gafter Dec 27, 2019 •

edited by 333fred

Loading

gafter Dec 27, 2019 •

edited

Loading

agocke Jan 2, 2020 •

edited

Loading

333fred commented Dec 31, 2019 •

edited

Loading